crypto: fix handling of null BUF_MEM* in ToV8Value()#61885
crypto: fix handling of null BUF_MEM* in ToV8Value()#61885ndossche wants to merge 2 commits intonodejs:mainfrom
Conversation
The assignment to `bptr` calls `BIO_get_mem_ptr` which can fail and leave the `bptr` as nullptr. This then later causes a null pointer deref. This is inconsistent with uses of the similar function `BIO_get_mem_data` that do check its return value, e.g. `node::crypto::X509sToArrayOfStrings()`. Solve it by checking for a null pointer and handling the `Nothing` return value at the call sites.
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61885 +/- ##
==========================================
+ Coverage 89.72% 89.74% +0.02%
==========================================
Files 675 674 -1
Lines 204806 204823 +17
Branches 39355 39368 +13
==========================================
+ Hits 183761 183821 +60
+ Misses 13330 13277 -53
- Partials 7715 7725 +10
🚀 New features to boost your workflow:
|
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
The BUF_MEM* changes are fundamentally reasonable, although they are a strong argument why they really should not be using implicit casting operators. However, both looking at the OpenSSL source code and documentation, there seems to be no indication that these would be able to fail.
The ToV8Value() changes are definitely incorrect; if ToV8Value() does not already schedule an exception to be thrown when returning an empty value, that's a bug in ToV8Value() itself.
I suppose it depends. The OpenSSL documentation is incomplete as usual and doesn't indicate what the return value means. Looking at the source, a registered callback could still trigger a failure. LibreSSL puts the failure more explicitly: https://man.openbsd.org/BIO_s_mem.3 where it indicates "return 1 on success or a value less than or equal to 0 if an error occurred." |
|
@ndossche Yeah, and like I said, it's a fundamentally reasonable change. My "request changes" bit primarily refers to the |
4 participants
The assignment to
bptrcallsBIO_get_mem_ptrwhich can fail and leave thebptras nullptr. This then later causes a null pointer deref.This is inconsistent with uses of the similar function
BIO_get_mem_datathat do check its return value, e.g.node::crypto::X509sToArrayOfStrings().Solve it by checking for a null pointer and handling the
Nothingreturn value at the call sites.Note: this was found by a static-dynamic analyser I'm developing.